Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert double script to return array #61504

Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 24, 2020

This replaces the value collection method in double valued runtime
script fields with simply returning a double[]. Painless has some
"convert" features that allow us to define automatic conversions from
things like double and Collection into double[] so we use those.

Relates to #59332

@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Aug 24, 2020
@nik9000 nik9000 requested a review from javanna August 24, 2020 20:14
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Aug 24, 2020
This replaces the value collection method in `double` valued runtime
script fields with simply returning a `double[]`. Painless has some
"convert" features that allow us to define automatic conversions from
things like `double` and `Collection` into `double[]` so we use those.
@@ -166,6 +166,9 @@ private static String painlessToLoadFromSource(String name, String type) {
return null;
}
StringBuilder b = new StringBuilder();
if ("double".equals(type)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if is temporary and it'll become all the time once it is done.

@@ -23,6 +23,8 @@ public AbstractLongScriptFieldScript(Map<String, Object> params, SearchLookup se
super(params, searchLookup, ctx);
}

public abstract void execute();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'm only doing one of them I have to move the old execute method decalarations into the subclasses.

@@ -36,7 +36,7 @@
/**
* Does the value match this query?
*/
protected abstract boolean matches(double[] values, int count);
protected abstract boolean matches(double[] values);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the count any more because we use the whole array.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a couple of questions. Shall we have unit tests for the static conversion methods? LGTM otherwise.

if (o instanceof Double) {
return convertFromDouble(((Double) o).doubleValue());
} else {
return (double[]) o;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boy I was hoping we would not need this sort of stuff, but I guess we do? I mean the instanceof as well as the cast to double array

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 Me too! I imagine the painless folk are thinking about it, but I'm not sure. This seems like a perfect spot for invokedynamic, but I'm not an expert at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we do without this conversion? Really bad I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanna without it you can't really return def. You get a lot of funny class cast exceptions.

@stu-elastic or @jdconrad do we have plans indy-ify this or something? So it'd call the converter based on the def type. Is that what #61389 is all about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed to get this in your hands ASAP. We'll be improving it: #61389

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indy for this is really challenging. We are going to look into it, but allowing you to do def conversions this way ensures we have something for runtime fields now that doesn't use reflection invocation. I would recommend that this cover all numeric cases including byte through long as well. Check out something like DefMath.plus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. So if I implement convertFromDef returning a def type is all on me at the moment. I have to do what DefMath does, basically.

Copy link
Contributor

@jdconrad jdconrad Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik9000 Yes, that's correct. Again we are going to rectify this, but wanted to make sure we had something that was usable now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘

}

# This import is required to make painless happy and it isn't 100% clear why
# This whitelist is required to allow painless to build the factory
class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Factory @no_import {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping that this file would go away completely. can you remind what the remaining lines are for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try one more time to drop it. I tried and it blew up. But I'll try without it entirely.

I think these are required so painless can use the classes that we defined. But these might should all be "implied" because they are part of the script context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked - it is still required.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stu-elastic and @jdconrad - I think we still need this file because without it painless can't find the factory class. I think that is because is part of a plugin and not in painless's classloader. Or something like that. Is that how the world is supposed to be? Could these classes be implied by default because the context mentions them directly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Painless has no way to load these classes w/o explicitly stating them as part of SPI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍.

Could you dig them out of the ScriptContext, I wonder?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something worth looking into. Agreed.

@javanna javanna mentioned this pull request Aug 25, 2020
30 tasks
*/
public final double[] values() {
return values;
public static double[] convertFromDouble(double v) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stu-elastic and @jdconrad do these look right? I borrowed them from FactoryTests.

I see right now you force the converters to be static - would it be possible to make them non-static on the script?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily because the only "this" pointer we have is through class bindings.

Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please address the one important note about convertFromDef.

if (o instanceof Double) {
return convertFromDouble(((Double) o).doubleValue());
} else {
return (double[]) o;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indy for this is really challenging. We are going to look into it, but allowing you to do def conversions this way ensures we have something for runtime fields now that doesn't use reflection invocation. I would recommend that this cover all numeric cases including byte through long as well. Check out something like DefMath.plus.

}

# This import is required to make painless happy and it isn't 100% clear why
# This whitelist is required to allow painless to build the factory
class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Factory @no_import {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Painless has no way to load these classes w/o explicitly stating them as part of SPI.

@nik9000 nik9000 merged commit 6d8170c into elastic:feature/runtime_fields Aug 25, 2020
@nik9000
Copy link
Member Author

nik9000 commented Aug 25, 2020

Thanks all!

nik9000 added a commit that referenced this pull request Aug 31, 2020
This reverts commit 6d8170c. We've
decided to stick with the `value` style functions for runtime fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants